-
Notifications
You must be signed in to change notification settings - Fork 107
[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @ViktorTigerstrom, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request is a foundational step in transitioning the firewalldb
to a SQL-based system. It focuses on implementing the intricate migration process for 'action' records, ensuring that historical data is accurately transferred and correctly associated with related sessions and accounts. The current changes are primarily focused on the migration mechanism and its robust validation through testing, laying the groundwork for future production deployment.
Highlights
- Action Data Migration: Introduced comprehensive logic to migrate 'action' data from the existing KV (BoltDB) store to the new SQL database within the
firewalldb
component. This includes handling various scenarios for linking actions to sessions and accounts. - New SQL Queries and Interfaces: Added new SQL queries and corresponding Go interfaces/implementations for
GetAction
(to retrieve action details from SQL) andUpdateAccountAliasForTests
(a utility for testing account alias updates). - Extensive Test Coverage for Migration Logic: Significantly expanded the test suite for database migration, including numerous scenarios to validate the correctness of action migration. These tests cover cases with no linked entities, linked sessions, linked accounts, filtered entities, and complex scenarios involving multiple colliding entities.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR introduces the migration logic for actions from kvdb
to SQL. The changes are extensive and include new SQL queries, migration logic, and a comprehensive set of tests. The migration logic correctly handles various scenarios of linking actions to sessions and accounts, with good heuristics for resolving ambiguities. The test suite is thorough, covering many edge cases. I've found a few issues, mostly minor comment discrepancies and a couple of bugs in the test code that prevent certain scenarios from being tested. Overall, this is a solid piece of work.
The upcoming commit will update the `AddActionReq` struct to include an extra field which the `kvdb` actions store will ignore. Therefore the `assertEqualActions` for the `kvdb` version will need to be update to ignore this field. In preparation for that change, we also do another optimization of the `assertEqualActions` function under kvdb builds, to not mutate the passed action references.
When migrating the actions store from kvdb to sql, we will update the existing actions to include the full mac root key, instead of just the last 4 bytes (currently called `MacaroonIdentifier`). In order to do so, we change the sql implementation of the `actions` store to persist the full mac root key, instead of just the last 4 bytes. As no production data in the sql actions store exists for users yet, it's fine for us to change this without having to address old sql actions which only stored the last 4 bytes. Note though that we do not update the kvdb implementation, and the full macaroon root key will be ignored by the kvdb store even if set. Therefore, the rest of `litd` will still have to just expect the last 4 bytes of the mac root key when accessing an `Action`'s MacaroonIdentifier. Therefore, we we currently never expose the rest of the mac root key outside of the sql actions store. Once the kvdb store has been fully deprecated and removed, we can then update the rest of `litd` to also use the full mac root key, and change the `Action` struct's field to reflect this.
Add a new SQL query `GetAction` to retrieve a single action by its ID. This query will be needed for the kvdb to SQL migration of actions store.
In the upcoming kvdb to SQL migration of the actions store, we need to simulate in tests that two or more accounts have colliding account aliases for the first 4 bytes of the alias. In order to allow creation of such accounts, we need to be able to update the alias of an account in tests, and this commit adds the a SQL query enabling this functionality. Note that the `UpdateAccountAliasForTests` query is only intended for use in tests and should not be used in production code.
In preparation for the kvdb to SQL migration of the actions store, this commit adds an `actions` field to the expected result of the migration tests. Once the migration is implemented, this field will be used to validate that the migrated actions match the expected results.
This commit adds an `accountStore` and a `rootKeyStore` arg the database population functions of the kvdb to SQL migration tests of the firewalldb. As an action can be linked to an account, we need to enable simulation of that in the migration tests of the actions store. In order to create the accounts to link the actions to, we need to create the accounts in the account store, which therefore requires passing the `accountStore` to database population functions of the migration tests. As the kvdb to SQL migration also will update the migrated actions to not only store the 4 byte short ID of the action's corresponding macaroon, but to it's full 8 byte root key ID. This requires the migration function has access to all of lnd's 8 byte root key IDs, and the migration function will therefore be change to accept a [][]byte arg containing all of lnd's root key IDs. As we can't access a full lnd instance in the migration unit tests, we need to create a mock instance that simulates the root key store, and this commit therefore adds mock `rootKeyStore` struct which is also passed to the database population functions of the migration tests. This `rootKeyStore` struct can be used to generate dummy root key IDs when creating simulated actions in the migration tests.
This commit introduces the migration logic for transitioning the actions store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.
2bd9a9c
to
2682b91
Compare
@ViktorTigerstrom, remember to re-request review from reviewers when ready |
Based on #1146
This PR introduces the migration logic for transitioning the actions from kvdb to SQL.
Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.
Part of #917
I will cleanup the code a little bit and add some more details to the commit messages before requesting reviews.